-
Notifications
You must be signed in to change notification settings - Fork 32
support update paypal accounts & custom params for list operation #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
wmews-hw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dfreudenberger,
We don't have a schedule defined for server SDKs. It is basically on-demand.
Can you please address my comments before we move forward?
| * @param params Custom filter options | ||
| * @return HyperwalletList of HyperwalletUser | ||
| */ | ||
| public HyperwalletList<HyperwalletUser> listUsers(HyperwalletPaginationOptions options, Map<String, String> params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide to me the use case so I can better understand why a generic params map is meant for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. According to the documentation each "list endpoint" supports different query params to filter the result. I.e. the list users endpoint supports the email parameter while the list bank accounts endpoint supports the type parameter. In order to find a specific user the best / most performant way to find the user would be filtering by email address which is not supported by the current implementation. Listing all users and filtering client side seems like a huge waste of resources.
We could have well defined query classes for each endpoint only exposing the supported params. If it'd be up to me only I would go with the map implementation since it allows developers to use parameters that might have been introduced to the api but are not yet included in the the query classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the design we have today, I'd prefer creating a HyperwalletUserListOptions that extends HyperwalletPaginationOptions.
I get when you say that this is way more flexible, but we want to keep our SDK self contained without requiring the developer to go check our documentation to understand what is available to be used and what isn't. That also helps us unit test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me. I'll pick it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check again. I'm not super happy with the current implementation but tried to prevent unnecessary changes.
My idea would be:
- introduce
getParametersintoHyperwalletPaginationOptions- to make this change happen, the date conversion needs to be moved from
HyperwallettoHyperwalletPaginationOptions
- to make this change happen, the date conversion needs to be moved from
HyperwalletUserListOptionswould overridegetParameters, take the parent params and add its ownpaginateoraddParametersinHyperwalletcould be replaced with one of the others
| return this; | ||
| } | ||
|
|
||
| public Map<String, String> getParameters() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did like @dfreudenberger but I will ask you to stick with the current design. We are planning to do changes on all our server SDKs to support the changes in our REST v4 and I will keep it in mind to implement for all resources.
|
@dfreudenberger please let me know if you can do that so we can merge and close this pull request. Again, we really appreciate your effort expading our capabilities. |
Hi,
any chance to get the PR integrated into the master? How long does it usually take to get a new release out.
If more changes are required (i.e. support custom params for all list operations) please let me know.
Best,
Daniel